Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(stack-packs): added draft for stack packs #7243

Merged
merged 29 commits into from
Apr 11, 2019
Merged

Conversation

wardpeet
Copy link
Collaborator

@wardpeet wardpeet commented Feb 13, 2019

Summary
This is a draft of stack packs to show how the integration will look like and to discuss some pain points before we actually implement it.

I've added a stack-packs.js file inside lib that creates the lhr result for stackpacks. It detects these stack-packs from LH artifacts.

The draft design of the stack pack result looks like:

export interface StackPacks {
  id: string;
  icon: string;
  detectedLibraries: string[];
  advice: Record<string, string>;
}

this would map to

{
  "id": "WordPress",
  "icon": "data:image/...",
  "detectedLibraries": ["WordPress"],
  "advice": {
    "unused-css-rules": "Consider reducing, or switching, ...",
    "uses-webp-images": "Consider using a plugin/service...",
  }
}

note, this ended up as
interface StackPack {
  /** The unique string ID for this stack pack. */
  id: string;
  /** The title of the stack pack, to be displayed in the report. */
  title: string;
  /** A base64 data url to be used as the stack pack's icon. */
  iconDataURL: string;
  /** A set of descriptions for some of Lighthouse's audits, keyed by the audits' `id`. */
  descriptions: Record<string, string>;
}

A put up a small demo up:
http://wardpeet-filestorage.surge.sh/lh-store/pr-7243/report.html
image

Related Issues/PRs
#7021

Copy link
Collaborator

@patrickhulce patrickhulce left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice!

lighthouse-core/lib/stack-packs.js Outdated Show resolved Hide resolved
lighthouse-core/lib/stack-packs.js Outdated Show resolved Hide resolved
lighthouse-core/lib/stack-packs.js Outdated Show resolved Hide resolved
lighthouse-core/report/html/renderer/util.js Outdated Show resolved Hide resolved
lighthouse-core/lib/stack-packs.js Outdated Show resolved Hide resolved
lighthouse-core/lib/stack-packs.js Outdated Show resolved Hide resolved
lighthouse-core/lib/stack-packs.js Outdated Show resolved Hide resolved
lighthouse-core/lib/stack-packs.js Outdated Show resolved Hide resolved
lighthouse-core/lib/stack-packs.js Outdated Show resolved Hide resolved
types/lhr.d.ts Outdated Show resolved Hide resolved
lighthouse-core/lib/stack-packs.js Outdated Show resolved Hide resolved
lighthouse-core/lib/stack-packs.js Outdated Show resolved Hide resolved
@paulirish
Copy link
Member

talked this out with @patrickhulce ... Here's the proposal:

  1. We plan now for detections that won't be possible with js-lib-detector.
  2. We rename our JSLibraries artifact to Technologies, and we could add more items to the array based on our own evaluation of the networkRecords, etc. (Like for stack packs for Fastly, Cloudflare, etc)
  3. We retitle the current 'js-libraries' audit because it'll now include things beyond just js-libraries.
  4. We use the definitions below.

stack pack definition:

stackPacks: [
  { 
      id: 'wordpress'
      title: 'WordPress',
      iconDataUri: 'data:image/...',
      requiredTechnologies: ['js:wordpressapi', 'core:wordpress'], // see 'type' and 'id' below.
      advice: {
        'unused-css-rules': 'Consider reducing, or switching, the number of [WordPress plugins](https://wordpress.org/plugins/) loading unused CSS in your page. To identify plugins that are adding extraneous CSS, ........',
        'uses-webp-images': 'Consider using a plugin/service that will automatically convert your uploaded images to the optimal formats.',
        // ...
      }
  }
}

Technologies artifact

interface GathererArtifacts {
  // ...
  Technologies: DetectedTechnology[]
}

interface DetectedTechnology {
  type: 'js', // basically the source of the detection. js is js-library-detector.. 
              // and 'core' is any detections custom defined in lighthouse's Technologies artifact.
  id: string //  if (type=='js') lib.npmPkgName || lib.icon. 
             //  if (type=='core') whatever we think is decent.
  name: string // the display-friendly name of the technology
  version?: string // any version data we have
  npm?: string // npmPkgName if it exists
}

image

@wardpeet
Copy link
Collaborator Author

sounds good to me, let me cleanup this PR to get comments resolved

@brendankenny
Copy link
Member

brendankenny commented Feb 26, 2019

We rename our JSLibraries artifact to Technologies

Unless we're renaming these "TechPecks", it seems like it's confusing to call this anything but Stacks? "Stack" is already super overloaded, referring both to entire stacks and also parts of the entire stack.

iconDataUri

we just say URL now :)

@brendankenny
Copy link
Member

interface DetectedTechnology {
  type: 'js', // basically the source of the detection. js is js-library-detector.. 
              // and 'core' is any detections custom defined in lighthouse's Technologies artifact.

What is type communicating? If it's the type of stack, it seems like core needs to instead be more about the stack and not how it was detected. If it's how it was detected, it doesn't seem that important to have, but for debugging and whatever, maybe a different property name? (e.g. detector: 'js-library-detector', detector: 'core', etc)

@patrickhulce
Copy link
Collaborator

If it's how it was detected, it doesn't seem that important to have, but for debugging and whatever, maybe a different property name? (e.g. detector: 'js-library-detector', detector: 'core', etc)

Yeah it's changed to now be how it is detected, so +1 to renaming to detector or something 👍

@paulirish
Copy link
Member

it seems like it's confusing to call this anything but Stacks?

K. That works for me. requiredStack: ['...', '...']

What is type communicating?

another +1 for type -> detector.

@housseindjirdeh
Copy link
Contributor

FYI : Submitted a pull request to include WordPress detection to the library-detector (since we still plan to use it for V1)

lighthouse-core/report/html/renderer/util.js Outdated Show resolved Hide resolved
lighthouse-core/report/html/renderer/category-renderer.js Outdated Show resolved Hide resolved
lighthouse-core/lib/stack-packs.js Outdated Show resolved Hide resolved
lighthouse-core/report/html/renderer/util.js Outdated Show resolved Hide resolved
lighthouse-core/gather/gatherers/dobetterweb/stacks.js Outdated Show resolved Hide resolved
lighthouse-core/lib/stack-packs-test/wordpress.json Outdated Show resolved Hide resolved
lighthouse-core/lib/stack-packs-test/package.json Outdated Show resolved Hide resolved
lighthouse-core/lib/stack-packs.js Outdated Show resolved Hide resolved
lighthouse-core/report/html/renderer/category-renderer.js Outdated Show resolved Hide resolved
lighthouse-core/report/html/renderer/util.js Outdated Show resolved Hide resolved
lighthouse-core/report/html/renderer/util.js Outdated Show resolved Hide resolved
lighthouse-core/report/html/renderer/util.js Outdated Show resolved Hide resolved
lighthouse-core/lib/stack-packs.js Outdated Show resolved Hide resolved
lighthouse-core/lib/stack-packs.js Show resolved Hide resolved
Copy link
Member

@exterkamp exterkamp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think my blocking comments have been addressed, i.e. proto.

@wardpeet wardpeet changed the title feat(stack-packs): added draft for stack packs [WIP] feat(stack-packs): added draft for stack packs Apr 9, 2019
Copy link
Member

@brendankenny brendankenny left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have some more clean-up ideas for some of the comments and the Stack types, but the overall structures look good (and I took too long with my changes :).

I don't want to block #8154, etc, which are necessary for getting i18n going, so let's move forward. I fixed the cli snapshot test and we can merge when green, then proceed from there

@brendankenny brendankenny merged commit b374ea4 into master Apr 11, 2019
@brendankenny brendankenny deleted the feat/stack-packs branch April 11, 2019 05:11
@brendankenny
Copy link
Member

🎉 🎉

@brendankenny
Copy link
Member

just a warning for those using file:./ and yarn, as far as I can tell yarn copies the directory into node_modules/, and then any changes you make to that directory after that aren't reflected when you run things.

I've been deleting the @lighthouse/stack-packs entry from yarn.lock and then running yarn again to force yarn to recopy the directory, but maybe there's an easier way :)

@brendankenny brendankenny restored the feat/stack-packs branch April 11, 2019 05:15
@brendankenny brendankenny deleted the feat/stack-packs branch April 11, 2019 05:15
@patrickhulce
Copy link
Collaborator

does --check-files help?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants